Upstream support for AWS-LC #672
Conversation
src/openvpn/crypto_openssl.c
Outdated
| @@ -1398,7 +1398,7 @@ ssl_tls1_PRF(const uint8_t *seed, int seed_len, const uint8_t *secret, | |||
|
|
|||
| return ret; | |||
| } | |||
| #elif !defined(LIBRESSL_VERSION_NUMBER) && !defined(ENABLE_CRYPTO_WOLFSSL) | |||
| #elif !defined(LIBRESSL_VERSION_NUMBER) && !defined(ENABLE_CRYPTO_WOLFSSL) && !defined(OPENSSL_IS_AWSLC) | |||
There was a problem hiding this comment.
Since you have custom code later in the #elif, I would prefer to just move the #elif defined(OPENSSL_IS_AWSLC) above this statement so we can avoid adding an additional term to this statement.
| if (tls_server) | ||
| { | ||
| cnum = sk_X509_NAME_num(cert_names); | ||
| SSL_CTX_set_client_CA_list(ctx->ctx, cert_names); | ||
| } |
There was a problem hiding this comment.
This change seems weird. With the old code we add the client CA list and then use sk_X509_NAME_num get the number, while with this patch we change the order. I haven't look exactly into what sk_X509_NAME_num does right now since it is one of the annoying function where finding the man page is hard but doesn't this change the logic?
There was a problem hiding this comment.
Good catch!This change addresses a subtle behavioral difference between AWS-LC and OpenSSL regarding object ownership semantics in SSL_CTX_set_client_CA_list(ctx->ctx, cert_names).
-
OpenSSL Behavior:
- Stores a reference to the provided
cert_namesstack - cert_names remains valid after
SSL_CTX_set_client_CA_list
- Stores a reference to the provided
-
AWS-LC Behavior:
- Creates a copy of the parameter
cert_names(which is a stack of typeX509_NAME) and converts it to a stack ofCRYPTO_BUFFER(how we internally representX509_NAME, it's an opaque byte string). - Then frees the original passed in cert_names
- After
SSL_CTX_set_client_CA_list,cert_namesno longer points to valid memory
- Creates a copy of the parameter
The proposed changes reorder operations to getting the size of the stack before the set operation as opposed to after the set operation. No operations between the setter and stack size check modify cert_names. Therefore, the logical outcome should remain the same - and this would also handle the subtle behavioral difference in AWS-LC.
There was a problem hiding this comment.
Thanks for the explaination. Can you copy&paste that explaination into your commit message, so it doesn't get lost?
| @@ -1895,9 +1899,11 @@ tls_ctx_load_ca(struct tls_root_ctx *ctx, const char *ca_file, | |||
| } | |||
| sk_X509_INFO_pop_free(info_stack, X509_INFO_free); | |||
| } | |||
|
|
|||
|
|
|||
| int cnum; | |||
There was a problem hiding this comment.
The extra new line here makes our formatting check fail. You can check the "show output on standard output in the github actions to see the full diff."
This PR contains minor changes to OpenVPN to improve compatibility with AWS-LC. It also has a README to explain the build process. Opening the PR to gather feedback before I contribute it through gerrit. Thank you!
Thank you for your contribution
You are welcome to open PR, but they are used for discussion only. All
patches must eventually go to the openvpn-devel mailing list for review:
Please send your patch using git-send-email. For example to send your latest commit to the list:
For details, see these Wiki articles: